Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Rails 4.1 #244

Merged
merged 5 commits into from
May 27, 2017
Merged

Upgrade to Rails 4.1 #244

merged 5 commits into from
May 27, 2017

Conversation

Throne3d
Copy link
Contributor

I expect it to take a bit before we actually merge this, seeing as we only recently got to Rails 4.0, but thought I'd make some progress on it anyway.

@Throne3d
Copy link
Contributor Author

Throne3d commented May 24, 2017

Tests pass! The site has not yet spontaneously broken (… since I fixed tests) while I use it in debug!

I'll go poke around a bit more to figure out if it's actually sufficient.

Edit: Poked around a bit more and yep! Stuff is not obviously broken.

Throne3d added 4 commits May 24, 2017 19:34
This file is explicitly mentioned in the 4.0 -> 4.1 upgrade
and it says if you want to use the new JSON format to set
:hybrid here, to transparently migrate cookies; I'm not sure
if this file was just ignored before, and we weren't using
JSON, or what?

If I run the server without this change it crashes when I try
loading the site, and with this change it works.
(Fix recognizing galleries_icons relation with protected attributes)
@Throne3d Throne3d force-pushed the upgrade/rails-4.1 branch from 25c6bd0 to 18134a5 Compare May 24, 2017 18:38
@Throne3d Throne3d changed the title [WIP] Upgrade to Rails 4.1 Upgrade to Rails 4.1 May 24, 2017
@Throne3d
Copy link
Contributor Author

So this should probably be poked at a bit more manually before we go pushing it. I guess. But tests do seem to pass, and so I'm considering it no longer a WIP. (Annoyingly, the upgrade docs hardly mentioned these things, if it did at all. Oh well.)

@@ -10,7 +10,7 @@ gem 'browser'
gem 'coffee-rails'
gem 'exception_notification'
gem 'gon'
gem 'haml-rails', '~> 0.4.0'
gem 'haml-rails'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we have a specific HAML version for any particular reason?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -107,7 +107,7 @@ def posts_from_relation(relation, no_tests=true, with_pagination=true)
posts = posts.paginate(page: page, per_page: 25) if with_pagination
posts = posts.no_tests if no_tests

if (with_pagination && posts.total_pages <= 1) || posts.count <= 25
if (with_pagination && posts.total_pages <= 1) || posts.count(:all) <= 25
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise it calls COUNT(posts.*, board.name AS board_name, …) which triggers a syntax error. See rails/rails#10710

@Marri
Copy link
Contributor

Marri commented May 24, 2017 via email

@Throne3d
Copy link
Contributor Author

Hmm, I think the warning about 4.0 not being properly supported bug me more than the lack of "proper" strong params.

@@ -1,3 +1,3 @@
# Be sure to restart your server when you modify this file.

Rails.application.config.action_dispatch.cookies_serializer = :json
Rails.application.config.action_dispatch.cookies_serializer = :hybrid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my personal edification, whyfor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this commit message answers why? The details are basically "I'm not sure why this file existed before, I don't think it applied, this should be better?

"906de238867e9806c7d894d16ccc23937bd8fcd5

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

906de23

(Oops)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't see the commit message, thanks!

@Marri Marri merged commit b77b116 into master May 27, 2017
@Marri Marri deleted the upgrade/rails-4.1 branch May 27, 2017 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants